Skip to content

Comments

fix: update test scripts to run all tests and handle remote branch conflicts#16

Open
yellow-seed wants to merge 1 commit intomainfrom
fix/failed_actions_script
Open

fix: update test scripts to run all tests and handle remote branch conflicts#16
yellow-seed wants to merge 1 commit intomainfrom
fix/failed_actions_script

Conversation

@yellow-seed
Copy link
Owner

@yellow-seed yellow-seed commented Dec 28, 2025

Summary by CodeRabbit

  • New Features

    • Added automatic handling of existing remote branches during pull request creation to prevent conflicts.
  • Tests

    • Expanded test suite execution to cover all tests in the test directory.
    • Added comprehensive test coverage for pull request creation workflow.
  • Documentation

    • Updated test documentation with clarified examples and test coverage details.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 28, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

The pull request enhances CI/CD and testing infrastructure by modifying the test execution workflow to run all BAT tests, adding remote branch conflict resolution to the PR creation script, and introducing a comprehensive test suite for the PR creation workflow with extensive documentation updates.

Changes

Cohort / File(s) Summary
CI Workflow Configuration
.github/workflows/test-scripts.yml
Changed test execution path from single file (tests/download-mozc-artifacts.bats) to entire tests directory (tests/), enabling all BAT tests to run in the workflow.
PR Creation Script Enhancement
scripts/create-update-pr.sh
Added pre-push conflict avoidance logic: detects existing remote branches via git ls-remote, deletes remote branch if it exists, then proceeds with new branch push.
Test Documentation
tests/README.md
Updated test invocation examples with inline comments, added "Test Coverage" section describing create-update-pr.bats coverage, and reflected workflow changes showing all tests execution.
New Test Suite
tests/create-update-pr.bats
Comprehensive Bats test suite validating create-update-pr.sh functionality including branch naming, git identity, remote conflict resolution, checksum calculation, commit staging, artifact handling, version string edge cases, and error scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

A rabbit hops through branches tall,
Deleting conflicts before they fall—
Tests multiply like clover sweet, 🐰
Making workflows clean and neat! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: updating test scripts to run all tests and handling remote branch conflicts in the create-update-pr.sh script.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/failed_actions_script

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@yellow-seed yellow-seed requested a review from Copilot December 28, 2025 11:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the testing infrastructure and adds remote branch conflict handling to the PR creation workflow. The changes ensure all test files are executed during CI runs and prevent push failures when remote branches already exist from previous workflow runs.

Key changes:

  • Added comprehensive test coverage for the PR creation script
  • Implemented automatic deletion of existing remote branches before pushing
  • Updated test execution commands to run all test files instead of a single file

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/create-update-pr.bats New test file with 23 test cases covering PR creation, git operations, remote branch handling, and artifact validation
tests/README.md Updated documentation to reference new test file and changed commands to run all tests
scripts/create-update-pr.sh Added logic to detect and delete existing remote branches before pushing to prevent conflicts
.github/workflows/test-scripts.yml Changed test execution to run all test files in the tests directory

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +184 to +187
# Verify message contains key components
[[ "$expected_message" == *"Update Mozc to version 2.32.5994"* ]]
[[ "$expected_message" == *"ARM64 (Apple Silicon) only support"* ]]
[[ "$expected_message" == *"google/mozc commit abc123def456"* ]]
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expected_message variable contains a multi-line string with list items using hyphens, but the assertions only verify partial matches. Consider using a more robust comparison that validates the complete structure or individual lines to ensure the commit message format is exactly as expected.

Suggested change
# Verify message contains key components
[[ "$expected_message" == *"Update Mozc to version 2.32.5994"* ]]
[[ "$expected_message" == *"ARM64 (Apple Silicon) only support"* ]]
[[ "$expected_message" == *"google/mozc commit abc123def456"* ]]
full_expected_message="Update Mozc to version $VERSION
- Updated cask formula with version $VERSION
- ARM64 (Apple Silicon) only support
- Source: google/mozc commit $COMMIT_SHA
- Artifacts from google/mozc GitHub Actions build"
# Verify the complete message structure matches exactly
[ "$expected_message" = "$full_expected_message" ]

Copilot uses AI. Check for mistakes.
Comment on lines +318 to +322
# Verify PR body contains key information
[[ "$pr_body" == *"Update Mozc to version 2.32.5994"* ]]
[[ "$pr_body" == *"ARM64 (Apple Silicon) only support"* ]]
[[ "$pr_body" == *"https://github.com/google/mozc/commit/abc123def456"* ]]
[[ "$pr_body" == *"v2.32.5994"* ]]
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the commit message test, this PR body validation only checks for partial string matches rather than verifying the complete structure. Consider adding assertions for specific sections (Changes, Artifacts, Source) to ensure the full template is properly formatted.

Suggested change
# Verify PR body contains key information
[[ "$pr_body" == *"Update Mozc to version 2.32.5994"* ]]
[[ "$pr_body" == *"ARM64 (Apple Silicon) only support"* ]]
[[ "$pr_body" == *"https://github.com/google/mozc/commit/abc123def456"* ]]
[[ "$pr_body" == *"v2.32.5994"* ]]
# Verify PR body contains structured sections
changes_section=$'### Changes\n- Updated version to '"$VERSION"$'\n- ARM64 (Apple Silicon) only support\n- Included build artifacts from google/mozc commit ['"$COMMIT_SHA"$'](https://github.com/google/mozc/commit/'"$COMMIT_SHA"$')\n'
artifacts_section=$'### Artifacts\n- `Mozc_arm64.pkg` (ARM64/Apple Silicon)\n'
source_section=$'### Source\nBuilt from google/mozc GitHub Actions: https://github.com/google/mozc/commit/'"$COMMIT_SHA"$'\n'
[[ "$pr_body" == *"$changes_section"* ]]
[[ "$pr_body" == *"$artifacts_section"* ]]
[[ "$pr_body" == *"$source_section"* ]]

Copilot uses AI. Check for mistakes.
@yellow-seed yellow-seed marked this pull request as ready for review December 28, 2025 11:36
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
tests/create-update-pr.bats (2)

184-187: Consider more robust commit message validation.

The test uses partial string matching rather than validating the complete structure or individual lines. This approach was flagged in previous reviews as potentially missing format issues.

As suggested in previous reviews, consider validating the complete message structure or verifying each line individually to ensure the commit message format is exactly as expected.


318-322: Consider more robust PR body validation.

Similar to the commit message test, this validation only checks for partial string matches rather than verifying the complete structure. Previous reviews suggested validating specific sections to ensure the full template is properly formatted.

As noted in previous reviews, consider adding assertions for specific sections (Changes, Artifacts, Source) with their complete expected content to ensure the PR body template is correctly formatted.

🧹 Nitpick comments (3)
tests/create-update-pr.bats (3)

1-22: Consider enhancing file header documentation.

Per coding guidelines, the file header could include more comprehensive documentation:

  • Overview of testing approach (mock file system for remote branch simulation)
  • Dependencies (bats, git)
  • Test environment setup details
🔎 Example enhanced header
 #!/usr/bin/env bats
 
-# Tests for create-update-pr.sh
-# This test file validates the PR creation and git operations logic
+# ========== Test Suite: create-update-pr.sh ==========
+#
+# Purpose: Validates PR creation workflow and git operations
+#
+# Overview:
+#   - Tests branch naming, git configuration, and artifact handling
+#   - Uses mock file system to simulate remote branch operations
+#   - Validates commit messages, PR body generation, and error handling
+#
+# Dependencies:
+#   - bats-core: Test framework
+#   - git: Version control operations
+#
+# Test Approach:
+#   - Creates temporary git repository for each test
+#   - Uses .remote_branches directory to mock remote state
+#   - Tests logic in isolation rather than full script integration

Based on coding guidelines for shell scripts.


31-54: Helper functions use mocking rather than actual git commands.

The helper functions simulate remote branch operations using a mock file system (.remote_branches directory) rather than testing actual git ls-remote and git push --delete commands. This unit testing approach validates the logic in isolation but doesn't verify that the actual git commands work as expected.

Consider adding integration tests that execute the actual create-update-pr.sh script with a real remote repository (e.g., using a local bare git repository as a mock remote). This would catch issues like:

  • Git command syntax errors
  • Authentication/permission problems
  • Network error handling
  • Exit code handling from git commands

The current mock-based tests are still valuable for fast unit testing of logic, so both approaches could coexist.

Would you like me to generate an example integration test setup using a local bare repository as a mock remote?


56-323: Consider adding section headers for improved organization.

Per coding guidelines for shell scripts, section headers like # ========== Section Name ========== can improve readability by demarcating main code blocks. With 19 test cases, grouping them into logical sections would make the file easier to navigate.

Example organization with section headers
# ========== Branch Management Tests ==========
@test "generates correct branch name from version" { ... }
@test "creates new branch correctly" { ... }
@test "handles version strings with dots and numbers correctly" { ... }

# ========== Remote Branch Conflict Resolution Tests ==========
@test "detects when remote branch exists" { ... }
@test "detects when remote branch does not exist" { ... }
@test "deletes remote branch when it exists" { ... }
@test "handles remote branch conflict by deleting existing branch" { ... }

# ========== Git Configuration Tests ==========
@test "configures git with bot identity correctly" { ... }

# ========== Artifact Handling Tests ==========
@test "validates artifacts directory structure" { ... }
@test "stages correct files for commit" { ... }
@test "calculates SHA256 checksum correctly" { ... }

# ========== Message Generation Tests ==========
@test "generates correct commit message format" { ... }
@test "generates PR body with correct information" { ... }

# ========== Cleanup and Navigation Tests ==========
@test "removes temporary PR body file after use" { ... }
@test "returns to original branch after operations" { ... }

# ========== Error Handling Tests ==========
@test "detects missing ARM64 package file" { ... }

Based on coding guidelines for shell scripts.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f1b4627 and caccf27.

📒 Files selected for processing (4)
  • .github/workflows/test-scripts.yml
  • scripts/create-update-pr.sh
  • tests/README.md
  • tests/create-update-pr.bats
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh

📄 CodeRabbit inference engine (AGENTS.md)

**/*.sh: Write appropriate granularity of explanatory comments in shell scripts to enhance code readability and maintainability
Include file header comments with script purpose, overview, usage instructions, and required environment variables/parameters
Use section headers like # ========== Section Name ========== to demarcate main processing stages and code blocks
Document shell functions with their purpose, arguments explanation, return values description, and usage examples for complex cases
Explain complex logic in shell scripts including condition branch intent, loop processing purpose, and regex/command pipeline operations
Document important configuration values and constants, explaining magic numbers and configuration choice rationale
Use #!/usr/bin/env bash shebang for improved portability across different systems
Implement appropriate error handling with error checks and messages for critical operations in shell scripts
Quote variables in shell scripts using double quotes (e.g., "$variable") to prevent word splitting and globbing issues
Use functions in shell scripts to encapsulate repeated operations for improved reusability and maintainability

Files:

  • scripts/create-update-pr.sh
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: yellow-seed/brew_mozc PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T12:39:16.748Z
Learning: Follow a development workflow: confirm Issue understanding, create feature/fix branches, implement changes, run tests, create Pull Requests, and merge after review
🔇 Additional comments (3)
tests/README.md (2)

64-70: LGTM! Clear and helpful examples.

The inline comments clarify the purpose of each command, and the examples properly demonstrate both running specific test files and all tests with verbose output.


88-103: LGTM! Comprehensive test coverage documentation.

The new section thoroughly documents all test scenarios covered by create-update-pr.bats, following the same format as the existing test documentation. The inclusion of remote branch conflict handling in the coverage list is particularly helpful.

.github/workflows/test-scripts.yml (1)

55-55: LGTM! Proper test execution scope.

Changing from running a single test file to running all tests in the directory aligns well with the new comprehensive test suite being added. This ensures all test coverage is executed in CI.

Comment on lines +167 to +172
# Check if remote branch exists and delete it to avoid conflicts
# This handles cases where a previous workflow run created the branch
if git ls-remote --exit-code --heads origin "$BRANCH_NAME" &> /dev/null; then
echo "Remote branch $BRANCH_NAME already exists, deleting it"
git push origin --delete "$BRANCH_NAME" || true
fi
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Improve error handling for remote branch deletion.

The || true on line 171 suppresses all deletion failures, which means the script will continue even if the deletion fails due to authentication issues, permission problems, or network errors. When the subsequent push on line 175 fails with the same conflict, users won't understand why.

🔎 Proposed fix with explicit error handling
 # Check if remote branch exists and delete it to avoid conflicts
 # This handles cases where a previous workflow run created the branch
 if git ls-remote --exit-code --heads origin "$BRANCH_NAME" &> /dev/null; then
   echo "Remote branch $BRANCH_NAME already exists, deleting it"
-  git push origin --delete "$BRANCH_NAME" || true
+  if ! git push origin --delete "$BRANCH_NAME"; then
+    echo "Warning: Failed to delete remote branch $BRANCH_NAME"
+    echo "The subsequent push may fail. Please check permissions and try again."
+    # Continue anyway in case the branch was deleted by another process
+  else
+    echo "Successfully deleted remote branch $BRANCH_NAME"
+  fi
 fi

Additionally, consider whether automatically deleting remote branches is safe in all scenarios. If multiple workflows or developers might be working with the same branch name, this could cause unexpected data loss.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Check if remote branch exists and delete it to avoid conflicts
# This handles cases where a previous workflow run created the branch
if git ls-remote --exit-code --heads origin "$BRANCH_NAME" &> /dev/null; then
echo "Remote branch $BRANCH_NAME already exists, deleting it"
git push origin --delete "$BRANCH_NAME" || true
fi
# Check if remote branch exists and delete it to avoid conflicts
# This handles cases where a previous workflow run created the branch
if git ls-remote --exit-code --heads origin "$BRANCH_NAME" &> /dev/null; then
echo "Remote branch $BRANCH_NAME already exists, deleting it"
if ! git push origin --delete "$BRANCH_NAME"; then
echo "Warning: Failed to delete remote branch $BRANCH_NAME"
echo "The subsequent push may fail. Please check permissions and try again."
# Continue anyway in case the branch was deleted by another process
else
echo "Successfully deleted remote branch $BRANCH_NAME"
fi
fi
🤖 Prompt for AI Agents
In scripts/create-update-pr.sh around lines 167-172, the remote-branch deletion
step currently swallows all errors via "|| true"; change this so failures are
detected and surfaced: remove the "|| true", capture the git push exit code, and
if non-zero print a clear error message including the git output and exit
non-zero to fail the workflow; optionally gate automatic deletion behind an
explicit env var (e.g. FORCE_DELETE_REMOTE=true) to avoid unintentional data
loss in multi-developer scenarios and document that behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant